Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Got FFT working again with new syntax. #1347

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

duvenaud
Copy link
Contributor

I like the new web interface! But it was a little sad adding extra type annotations. I was lucky to realize that an especially hairy type on line 126 could be elided by using each. But it's not clear to me why each is able to infer the type of the index to loop over when for isn't.

@dougalm
Copy link
Collaborator

dougalm commented Apr 29, 2024

Nice!

But it was a little sad adding extra type annotations.

Sorry about that. Right now I'm trying to simplify the implementation above all else, which means cutting features.

But it's not clear to me why each is able to infer the type of the index to loop over when for isn't.

I can explain this one. The new type inference is forward-only and very local (there are arguments for this besides simplifying the implementation but that's another matter). each takes a table as an argument which carries an index set. By the time we reach the lambda argument to each we've seen that table so we know the index set. But we don't learn the index set for the for until we see how the index is used. A general principle of the new inference system is that we never infer things about a variable based on how it's used. Type inference follows data flow. It's the UTI principle: type information shouldn't flow backwards.

But please keep the friction reports coming regardless. I want to at least be aware of the cost I'm imposing on users with these choices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants